Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BitBox02 Simulator #741

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

asi345
Copy link

@asi345 asi345 commented Jun 5, 2024

Recently, a simulator for BitBox02 was implemented. Therefore, we also wanted to add this to HWI repository for automated tests and ease of reference in the future.

@benma
Copy link
Contributor

benma commented Jun 6, 2024

fyi @achow101, I reviewed this before @asi345 made the PR

@@ -130,11 +129,12 @@ def create(*args, **kwargs):
return c

class DeviceTestCase(unittest.TestCase):
def __init__(self, bitcoind, emulator=None, interface='library', methodName='runTest'):
def __init__(self, bitcoind, emulator=None, interface='library', methodName='runTest', supports_legacy=True):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supports_legacy should be a property of the emulator, not an argument to the test case. See supports_taproot.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we can use this convention. I'll updating that shortly, but then I will also add the relevant field to the other emulators.

Comment on lines 586 to 587
keypool_desc = self.do_command(self.dev_args + ["getkeypool", "--account", "10", "--addr-type", "legacy", "0", "100"])
addr_type = "legacy" if self.supports_legacy else "sh_wit"
keypool_desc = self.do_command(self.dev_args + ["getkeypool", "--account", "10", "--addr-type", addr_type, "0", "100"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine to just change the address type to sh_wit always instead of using legacy for the other devices.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, changed the types to sh_wit

Comment on lines 445 to 448
docker pull shiftcrypto/firmware_v2:latest
# The safe.directory config is so that git commands work. even though the repo folder mounted in
# Docker is owned by root, which can be different from the owner on the host.
docker run -i --rm --volume ./:/bb02 shiftcrypto/firmware_v2 bash -c "git config --global --add safe.directory /bb02 && cd /bb02 && make -j simulator"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script needs to work when run inside a docker container as that is how the tests are suggested to be run locally. IIRC Docker within docker doesn't really work, so I don't think this will.

@asi345
Copy link
Author

asi345 commented Jul 17, 2024

Okay, we also came up with a solution for docker in docker problem for bitbox02 simulator.
Now, a separate build_bitbox02.sh script is ran before running hwi container, to build BitBox02 simulator.
See the comments in ci Dockerfile for reference.

test/README.md Outdated
Pull the BitBox02 firmware Docker image:

```
docker pull shiftcrypto/firmware_v2:latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with

cd bitbox02-firmware
make dockerpull

(remove the cd below then)

fi

# Build the simulator. This is cached, but it is also fast
docker pull shiftcrypto/firmware_v2:latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use $(cat .containerversion) instead of latest here too

@benma
Copy link
Contributor

benma commented Aug 22, 2024

@achow101 please approve the workflow and take another look at the changes, would be really nice to have this merged soon. Thanks!

@achow101
Copy link
Member

Could you rebase so that CI runs the tests with the simulator?

In order to build the BitBox02 simulator, the following packages will need to be installed:

```
apt install docker.io
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a way to build it without docker?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, this is how we build and release the BB02 firmware. Replicating the Docker environment outside of it is bound to lead to tons of overhead in maintenance and CI breaks.

podman instead of docker would also work though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, anyway, I could build and run it. No problem.

parser.add_argument('--interface', help='Which interface to send commands over', choices=['library', 'cli', 'bindist'], default='library')
args = parser.parse_args()

sys.exit(not bitbox02_test_suite(args.simulator, None, None))
Copy link
Contributor

@brunoerg brunoerg Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some problems with this test. It seems it should pass bitcoind and interface in bitbox02_test_suite instead of None, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. Fixed that now.

docker pull shiftcrypto/firmware_v2:$CONTAINER_VERSION
# The safe.directory config is so that git commands work. even though the repo folder mounted in
# Docker is owned by root, which can be different from the owner on the host.
docker run -i --rm --volume ./:/bb02 shiftcrypto/firmware_v2:$CONTAINER_VERSION bash -c "git config --global --add safe.directory /bb02 && cd /bb02 && make -j simulator"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got this error running setup_environment.sh:

+ docker run -i --rm --volume ./:/bb02 shiftcrypto/firmware_v2:42 bash -c 'git config --global --add safe.directory /bb02 && cd /bb02 && make -j simulator'
docker: Error response from daemon: create ./: "./" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed. If you intended to pass a host directory, use absolute path.

I think you could use "$(pwd):/bb02" instead of ./:/bb02.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not an issue in CI, but in local maybe that created problems. Anyways, I changed to $(pwd) instead of . as you suggested. This should solve the error.

@asi345
Copy link
Author

asi345 commented Sep 4, 2024

Okay, I rebased this branch on top of master. CI is again waiting for your approval.

However, can you in any chance fix bitcoind build first? This commit breaks bitcoind build in setup_environment.sh, since autogen.sh file is not existent in bitcoin repo anymore : bitcoin/bitcoin@d71ac76

@brunoerg
Copy link
Contributor

brunoerg commented Sep 4, 2024

However, can you in any chance fix bitcoind build first? This commit breaks bitcoind build in setup_environment.sh, since autogen.sh file is not existent in bitcoin repo anymore : bitcoin/bitcoin@d71ac76

See #752

docker volume create bitbox02_volume
CONTAINER_VERSION=$(curl https://raw.githubusercontent.com/BitBoxSwiss/bitbox02-firmware/master/.containerversion)
docker pull shiftcrypto/firmware_v2:$CONTAINER_VERSION
docker run -i --rm -v bitbox02_volume:/bitbox02-firmware shiftcrypto/firmware_v2:$CONTAINER_VERSIONs bash -c \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
docker run -i --rm -v bitbox02_volume:/bitbox02-firmware shiftcrypto/firmware_v2:$CONTAINER_VERSIONs bash -c \
docker run -i --rm -v bitbox02_volume:/bitbox02-firmware shiftcrypto/firmware_v2:$CONTAINER_VERSION bash -c \

def __init__(self) -> None:
self.client_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
ip, port = SIMULATOR_PATH.split(":")
self.client_socket.connect((ip, int(port)))
Copy link
Contributor

@brunoerg brunoerg Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It throws ConnectionRefusedError if it wasn't able to connect to the BitBox02 simulator, so we won't be able to use any other simulator.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I saw, Trezor or Keepkey also call connect on their simulators without handling any exceptions or connect_ex. Do you have a suggestion how to handle this exactly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I saw, Trezor or Keepkey also call connect on their simulators without handling any exceptions or connect_ex.

See:

def enumerate(password: Optional[str] = None, expert: bool = False, chain: Chain = Chain.MAIN, allow_emulators: bool = False) -> List[Dict[str, Any]]:
    results = []
    devs = hid.HidTransport.enumerate()
    devs.extend(webusb.WebUsbTransport.enumerate())
    if allow_emulators:
        devs.extend(udp.UdpTransport.enumerate())
    for dev in devs:

When calling enumerate with --simulators, Trezor will call enumerate from UdpTransport. This function will return nothing if it wasn't able to find a device (simulator) so devs is empty.

Maybe you could do something like:

diff --git a/hwilib/devices/bitbox02.py b/hwilib/devices/bitbox02.py
index 53497c6..7cd6929 100644
--- a/hwilib/devices/bitbox02.py
+++ b/hwilib/devices/bitbox02.py
@@ -186,6 +186,8 @@ def enumerate(password: Optional[str] = None, expert: bool = False, chain: Chain
         devs.append(SIMULATOR_PATH)
     for path in devs:
         client = Bitbox02Client(path=path)
+        if allow_emulators and not client.transport:
+            continue
         if path != SIMULATOR_PATH:
             client.set_noise_config(SilentNoiseConfig())
         d_data: Dict[str, object] = {}
@@ -267,7 +269,11 @@ class BitBox02Simulator():
     def __init__(self) -> None:
         self.client_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
         ip, port = SIMULATOR_PATH.split(":")
-        self.client_socket.connect((ip, int(port)))
+        self.connected = True
+        try:
+            self.client_socket.connect((ip, int(port)))
+        except:
+            self.connected = False
 
     def write(self, data: bytes) -> None:
         # Messages from client are always prefixed with HID report ID(0x00), which is not expected by the simulator.
@@ -301,7 +307,10 @@ class Bitbox02Client(HardwareWalletClient):
             self.noise_config = CLINoiseConfig()
         else:
             simulator = BitBox02Simulator()
-            self.transport = u2fhid.U2FHid(simulator)
+            if simulator.connected:
+                self.transport = u2fhid.U2FHid(simulator)
+            else:
+                self.transport = None
         self.device_path = path
 
         # use self.init() to access self.bb02.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I did a fix based on this(not the same since this does not pass type checks). Now, in my tests, other devices are also passing the tests.

@achow101
Copy link
Member

The current implementation is causing all other tests to fail.

@asi345
Copy link
Author

asi345 commented Sep 25, 2024

I updated the commit to handle connection refused on BitBox02 simulator, now it seems that other devices do not fail.

@benma
Copy link
Contributor

benma commented Oct 28, 2024

The current implementation is causing all other tests to fail.

@achow101 please take a look again, @asi345 updated the PR and all checks have passed.

@benma
Copy link
Contributor

benma commented Nov 27, 2024

@achow101 ping again - it's so close 🤓 😄 would be great to merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants